Conversation
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utils/methods.js (1)
305-311: Consolidate emptiness helpers to avoid semantic drift.Line 305 adds a second exported helper while
isEmptyalready exists (Line 213). Keeping both increases inconsistent usage risk; prefer one canonical helper name/behavior and migrate callers over time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/methods.js` around lines 305 - 311, The new exported helper empty duplicates existing isEmpty causing inconsistent usage; replace the implementation/export of empty with a canonical alias to isEmpty (or remove the separate definition and export isEmpty only) so both names resolve to the same logic; update the symbol references by changing the export of empty to reference isEmpty (keeping a one-line alias export) or delete the empty function and update callers to import isEmpty instead, and ensure only the isEmpty implementation (the function at or around the existing isEmpty declaration) remains as the single source of truth.src/components/mui/dropdown-checkbox.js (1)
52-53: Consider collapsing duplicated"all"emission logic.Line 53 sends the same payload as Line 46. You can reduce branching by emitting
["all"]once in a shared path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/dropdown-checkbox.js` around lines 52 - 53, The component currently emits identical payloads of ["all"] in two branches; consolidate that logic by calling onChange({ target: { name, value: ["all"] } }) from a single shared path instead of duplicating it. Locate the handler using onChange and the relevant variables name and value (e.g., inside the component's change handler or function that processes selections) and refactor so both branches route to the same "emit all" call (remove the duplicate call and ensure any branch-specific cleanup happens before the shared emit).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/mui/dropdown-checkbox.js`:
- Around line 36-41: Normalize the types of items in selected when converting
ev.target.value so downstream comparisons like selected.includes(id) and
value.includes(id) match the option id types: after splitting the string
fallback, map the selected array to the same type as your option ids (e.g.
detect typeof options[0].id — if it's "number" convert numeric-looking strings
with Number(...) or parseInt, otherwise leave strings) so comparisons against id
work correctly; update the code that builds selected from ev.target.value (the
rawValue -> selected conversion) to perform this normalization.
In `@src/components/mui/SnackbarNotification/index.js`:
- Around line 63-65: The local state msgData isn't cleared when
snackbarMessage.html is reset to "", so update the handler around
snackbarMessage (the code using snackbarMessage?.html and setMsgData) to
synchronize clears: always call setMsgData(snackbarMessage) (or call
setMsgData(null) / {} in the else branch) when snackbarMessage becomes empty or
falsy so CLEAR_SNACKBAR_MESSAGE from Redux clears the local msgData; adjust the
effect or handler that reads snackbarMessage to setMsgData(snackbarMessage)
unconditionally or add an explicit else path to clear msgData.
---
Nitpick comments:
In `@src/components/mui/dropdown-checkbox.js`:
- Around line 52-53: The component currently emits identical payloads of ["all"]
in two branches; consolidate that logic by calling onChange({ target: { name,
value: ["all"] } }) from a single shared path instead of duplicating it. Locate
the handler using onChange and the relevant variables name and value (e.g.,
inside the component's change handler or function that processes selections) and
refactor so both branches route to the same "emit all" call (remove the
duplicate call and ensure any branch-specific cleanup happens before the shared
emit).
In `@src/utils/methods.js`:
- Around line 305-311: The new exported helper empty duplicates existing isEmpty
causing inconsistent usage; replace the implementation/export of empty with a
canonical alias to isEmpty (or remove the separate definition and export isEmpty
only) so both names resolve to the same logic; update the symbol references by
changing the export of empty to reference isEmpty (keeping a one-line alias
export) or delete the empty function and update callers to import isEmpty
instead, and ensure only the isEmpty implementation (the function at or around
the existing isEmpty declaration) remains as the single source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7d23903-a655-442a-8370-8d820dc12ec0
📒 Files selected for processing (3)
src/components/mui/SnackbarNotification/index.jssrc/components/mui/dropdown-checkbox.jssrc/utils/methods.js
| const rawValue = ev.target.value; | ||
| const selected = Array.isArray(rawValue) | ||
| ? rawValue | ||
| : typeof rawValue === "string" | ||
| ? rawValue.split(",") | ||
| : []; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In MUI v6 Select with multiple, when can event.target.value be a string, and what does MUI recommend for handling value types when option values are numbers?
💡 Result:
-
With
Select multiple,event.target.valueis only astringwhen the change is coming from browser autofill (otherwise it’s the normal “generic event” MUI fires, and the value you read is the array). MUI explicitly calls out that theonChangeevent is not a change event unless caused by autofill. [1] The type definition reflects this by allowingSelectChangeEventto also be aReact.ChangeEvent<HTMLInputElement>(whosetarget.valueis a string). [2] -
If your option values are numbers, MUI’s guidance implies you should handle the fact that selection matching is done via string representation (so types must stay consistent), and therefore you should convert the autofill string back into the numeric type you store (e.g., split and
map(Number)for multiple). [1]
Sources: [1] Select API (onChange autofill note; value/string-representation matching), [2] SelectChangeEvent type (union including React.ChangeEvent<HTMLInputElement>)
🏁 Script executed:
cat -n src/components/mui/dropdown-checkbox.js | head -60Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 2253
🏁 Script executed:
cat -n src/components/mui/dropdown-checkbox.js | tail -n +60Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 1535
🏁 Script executed:
rg "DropdownCheckbox" --type js --type tsx -B 2 -A 5 | head -80Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 111
🏁 Script executed:
rg "DropdownCheckbox" --type js -B 2 -A 5 | head -100Repository: OpenStackweb/openstack-uicore-foundation
Length of output: 8121
🏁 Script executed:
cat -n src/components/mui/__tests__/dropdown-checkbox.test.jsRepository: OpenStackweb/openstack-uicore-foundation
Length of output: 2252
Normalize option ID types when converting ev.target.value in autofill scenarios.
When MUI Select fires onChange from browser autofill, ev.target.value is a string that must be split into an array. However, if option IDs are numeric, this creates a type mismatch: the selected array contains strings while the downstream checks at line 77 (selected.includes(id)) and line 89 (value.includes(id)) expect numeric types. This breaks both the rendered display and checked state after autofill.
Proposed fix
const handleChange = (ev) => {
const rawValue = ev.target.value;
- const selected = Array.isArray(rawValue)
- ? rawValue
- : typeof rawValue === "string"
- ? rawValue.split(",")
- : [];
+ const selected = (Array.isArray(rawValue)
+ ? rawValue
+ : typeof rawValue === "string"
+ ? rawValue.split(",")
+ : []
+ ).map((v) => {
+ if (v === "all") return "all";
+ const match = options.find(({ id }) => String(id) === String(v));
+ return match ? match.id : v;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const rawValue = ev.target.value; | |
| const selected = Array.isArray(rawValue) | |
| ? rawValue | |
| : typeof rawValue === "string" | |
| ? rawValue.split(",") | |
| : []; | |
| const rawValue = ev.target.value; | |
| const selected = (Array.isArray(rawValue) | |
| ? rawValue | |
| : typeof rawValue === "string" | |
| ? rawValue.split(",") | |
| : [] | |
| ).map((v) => { | |
| if (v === "all") return "all"; | |
| const match = options.find(({ id }) => String(id) === String(v)); | |
| return match ? match.id : v; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/mui/dropdown-checkbox.js` around lines 36 - 41, Normalize the
types of items in selected when converting ev.target.value so downstream
comparisons like selected.includes(id) and value.includes(id) match the option
id types: after splitting the string fallback, map the selected array to the
same type as your option ids (e.g. detect typeof options[0].id — if it's
"number" convert numeric-looking strings with Number(...) or parseInt, otherwise
leave strings) so comparisons against id work correctly; update the code that
builds selected from ev.target.value (the rawValue -> selected conversion) to
perform this normalization.
| if (!empty(snackbarMessage?.html)) { | ||
| setMsgData(snackbarMessage); | ||
| } |
There was a problem hiding this comment.
Redux clear events are no longer synced to local msgData.
Line 63 only updates local state when snackbarMessage?.html is non-empty. When CLEAR_SNACKBAR_MESSAGE resets html to "", local msgData is not cleared, so stale content can persist until another local action closes it.
🛠️ Proposed fix
useEffect(() => {
if (!empty(snackbarMessage?.html)) {
setMsgData(snackbarMessage);
+ } else {
+ setMsgData({});
}
}, [snackbarMessage]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!empty(snackbarMessage?.html)) { | |
| setMsgData(snackbarMessage); | |
| } | |
| if (!empty(snackbarMessage?.html)) { | |
| setMsgData(snackbarMessage); | |
| } else { | |
| setMsgData({}); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/mui/SnackbarNotification/index.js` around lines 63 - 65, The
local state msgData isn't cleared when snackbarMessage.html is reset to "", so
update the handler around snackbarMessage (the code using snackbarMessage?.html
and setMsgData) to synchronize clears: always call setMsgData(snackbarMessage)
(or call setMsgData(null) / {} in the else branch) when snackbarMessage becomes
empty or falsy so CLEAR_SNACKBAR_MESSAGE from Redux clears the local msgData;
adjust the effect or handler that reads snackbarMessage to
setMsgData(snackbarMessage) unconditionally or add an explicit else path to
clear msgData.
3 participants
ref: https://app.clickup.com/t/86b8t9qw1
Components: SnackbarNotification, DropdownCheckbox.
Summary by CodeRabbit
Bug Fixes
Enhancements